Rework get_flat_value to fix node relationships
#487
Conversation
WalkthroughThis change refactors the logic for flat notation value lookups (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Node (async/sync)
participant RelatedNode
Caller->>Node: extract(params)
loop For each key in params
Node->>Node: get_flat_value(key)
alt Key traverses relationship
Node->>RelatedNode: fetch (awaited or sync)
RelatedNode->>Node: get_flat_value(remaining_key)
end
Node-->>Caller: value
end
Node-->>Caller: dict of extracted values
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying infrahub-sdk-python with
|
| Latest commit: |
e2552e5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://66ba3368.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://gma-20250729-6882.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## stable #487 +/- ##
==========================================
+ Coverage 75.63% 75.75% +0.11%
==========================================
Files 100 100
Lines 8767 8821 +54
Branches 1714 1727 +13
==========================================
+ Hits 6631 6682 +51
- Misses 1660 1664 +4
+ Partials 476 475 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
infrahub_sdk/node/node.py (2)
1031-1062: Consider handling AttributeError in nested attribute access.The implementation correctly handles flat notation lookups. However, the nested
getattrcalls in lines 1045-1047 could raiseAttributeErrorif any intermediate attribute doesn't exist. Consider wrapping this in a try-except block for more graceful error handling.if first in self._schema.attribute_names: attr = getattr(self, first) - for part in remaining.split(separator): - attr = getattr(attr, part) - return attr + try: + for part in remaining.split(separator): + attr = getattr(attr, part) + return attr + except AttributeError as exc: + raise AttributeError(f"Invalid attribute path: {key}") from exc
1657-1688: Consider consistent error handling with async version.The sync implementation correctly mirrors the async logic. Similar to the async version, consider adding error handling for the nested
getattrcalls in lines 1670-1673.if first in self._schema.attribute_names: attr = getattr(self, first) - for part in remaining.split(separator): - attr = getattr(attr, part) - return attr + try: + for part in remaining.split(separator): + attr = getattr(attr, part) + return attr + except AttributeError as exc: + raise AttributeError(f"Invalid attribute path: {key}") from exc
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
changelog/6882.fixed.md(1 hunks)infrahub_sdk/node/node.py(3 hunks)infrahub_sdk/protocols_base.py(0 hunks)infrahub_sdk/utils.py(0 hunks)tests/unit/sdk/test_node.py(1 hunks)tests/unit/sdk/test_utils.py(0 hunks)
💤 Files with no reviewable changes (3)
- infrahub_sdk/protocols_base.py
- tests/unit/sdk/test_utils.py
- infrahub_sdk/utils.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit/sdk/test_node.py (2)
tests/unit/sdk/conftest.py (4)
clients(37-42)location_schema(140-172)location_data01(375-412)client(32-33)infrahub_sdk/node/node.py (6)
InfrahubNode(452-1078)get_flat_value(1031-1061)get_flat_value(1657-1687)InfrahubNodeSync(1081-1704)extract(1063-1069)extract(1689-1695)
🔇 Additional comments (6)
changelog/6882.fixed.md (1)
1-1: LGTM!The changelog entry clearly describes the fix for flat notation value lookups with cardinality-one relationships.
tests/unit/sdk/test_node.py (2)
1950-1976: LGTM!The test comprehensively covers the
get_flat_valuemethod for both async and sync node instances. It properly:
- Tests attribute access via flat notation (
name__value)- Tests relationship traversal with cardinality one (
primary_tag__display_label)- Tests custom separators (
.vs__)- Validates error handling for many-cardinality relationships
- Uses appropriate mocking with reusable responses
1978-1996: LGTM!The test properly validates the
extractmethod functionality for both async and sync node instances. It correctly:
- Tests multiple key extractions in a single call
- Uses flat notation for value lookup (
name__value,description__value)- Verifies the returned dictionary structure and values
- Follows consistent parametrized testing patterns
infrahub_sdk/node/node.py (3)
11-11: Import cleanup looks good.The removal of
get_flat_valuefrom utils import aligns with moving this functionality into the node classes.
1063-1069: Extract method implementation looks good.The async extract method correctly iterates through params and awaits the flat value lookups.
1689-1696: Sync extract method implementation is correct.The method properly implements the synchronous version of extract, maintaining consistency with the async implementation.
2 participants
This change moves the
get_flat_valuefunction to methods of theInfrahubNodeandInfrahubNodeSyncclasses. This allows to handle traversing relationships of cardinality one using a flat notation in addition to attributes.The
extractmethod has also been moved as it needs to beasyncfor regular nodes now.Summary by CodeRabbit
Bug Fixes
foo__bar__value) for relationships with cardinality one, ensuring correct resolution of nested attributes.New Features
Tests